child_process: ignore undef/proto values of env #15089

Gerhut · 2017-08-30T10:10:58Z

At present, undefined values of env option will be transferred as an "undefined" string value and values in the prototype will also be included, which are not usual behaviors.

Since non-string env values & prototype values are undocumented, this change may be treated as a bugfix or a breaking change.

Tested on Mac, Windows not yet.

Fixes: #15087

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

child_process

benjamingr · 2017-08-30T10:42:32Z

lib/child_process.js

@@ -467,7 +467,10 @@ function normalizeSpawnArguments(file, args, options) {
  var envPairs = [];

  for (var key in env) {


I think this should optimally be:

const envPairs = Object.keys(env).filter(Boolean).map(key => `${key}=${env[key]}`)

or something similar (ignore empty keys in addition to undefined ones.

I do think it's underspecified though. So I'll wait for someone who understands it better to weigh in.

OMG .filter(Boolean) 💘
I used to do .filter((x) => x), but have alway looked for a built-in.

P.S. @Gerhut in TF&I (node 8.3.0 and newer) for ... of loop are almost 10 time faster than for ... in loops, and it's supports Object.entries()

@refack but the call to Object.keys is itself about as slow as doing for... in - in this particular case I don't think performance is important given the time it takes to create a process and all 😅

@benjamingr agreed.
I'm just promoting TF&I good behaviour (no more CranksharfScript)

Since the code around still in legacy Syntax, should I be allowed to use modern syntax? If so I will modify them in modern syntax.

@Gerhut yes, in general new code should be written in new syntax.

However, for..in includes keys in prototype, and keys-in-prototype of env test exists at https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-env.js#L31-L33 . Changing to Object.keys/entries will fail them

I think it would be good to remove that test case and only use Object.keys. There is not much reasoning in the original commit about why it was added but I think this is actually a faulty behavior.

Using filter(Boolean) would be a bad decision to use though as it would remove a empty string and any other falsy value as well.

benjamingr · 2017-08-30T10:44:04Z

Tagging as semver-major preemptively since this is a user facing behavioral change in an API marked 2 - Stable.

I don't feel strongly about it - I just don't want it to be missed.

benjamingr · 2017-08-30T10:46:54Z

@Gerhut thank you for the contribution. Just to explain the process at this point:

Collaborators are going to look at this pull request for around 48 hours to discuss the change.
People will comment on the actual code and might suggest changes.
People will comment on the merit of the actual changes and evaluate how they impact the ecosystem.

If at any point you would like assistance with the pull request or have any question about the process feel free to ask here. Welcome.

cjihrig

Can you add documentation notes and function level changelog entries for this.

addaleax · 2017-08-31T00:34:37Z

LGTM as semver-major

BridgeAR · 2017-08-31T03:20:30Z

I personally am not convinced about this. Why should undefined and null be treated otherwise than any other primitive besides strings? They will all be coerced to a string and I can actually imagine that some people might explicitly pass undefined or null as env value. Having a implicit removal is not what I would call intuitive.
I would rather have a error thrown as a user to know what is happening and that it is better to always pass in values as string.

jasnell · 2017-09-01T16:24:31Z

I'm kind of half-way where @BridgeAR is. I would say null should be ToString'd and undefined should be ignored/omitted.

targos · 2017-09-01T18:15:52Z

I think it makes sense to ignore undefined too. No opinion on null.

addaleax · 2017-09-02T02:01:07Z

100 % totally official twitter poll: https://twitter.com/addaleax/status/903796395865452544

BridgeAR · 2017-09-03T22:54:01Z

Looks like the poll says null should be stringified and undefined should be removed. @Gerhut would you be so kind and update the PR accordingly?

I personally still think we should not remove it but throw an error instead and the poll did not cover that part but it seems like most people agree on removing it, so I will not hold this off.

refack · 2017-09-03T23:06:03Z

BTW: need to think about how to make this consistent with assignment (maybe in a future PR)

node/doc/api/process.md

Lines 827 to 842 in 98d8db3

    
           Assigning a property on `process.env` will implicitly convert the value 
        
           to a string. 
        
           Example: 
        
           ```js 
        
           process.env.test = null; 
        
           console.log(process.env.test); 
        
           // => 'null' 
        
           process.env.test = undefined; 
        
           console.log(process.env.test); 
        
           // => 'undefined' 
        
           ``` 
        
           Use `delete` to delete a property from `process.env`.

Gerhut · 2017-09-04T03:26:24Z

PR Updated.

Moreover, Would "ignore keys in prototype of env option" be another breaking change of bugfix? (See outdated review comment)

I just feel this change should be in another PR to discuss separately. There could be an use case that using process.env as a base environment variables and extend it in env option. It might using prototyping (env = Object.create(process.env); env.FOO='BAR'; spawn(exe, {env};).

BridgeAR

In general LGTM but I would like my comment to be addressed.

BridgeAR · 2017-09-11T04:18:22Z

lib/child_process.js

@@ -466,8 +466,10 @@ function normalizeSpawnArguments(file, args, options) {
  var env = options.env || process.env;
  var envPairs = [];

-  for (var key in env) {
-    envPairs.push(key + '=' + env[key]);
+  for (const [key, value] of Object.entries(env)) {


I am not sure how fast Object.entries is and I somewhat expect it to be slower than e.g. using Object.keys or using for ... in with a Object.hasOwnProperty.call(env, key) check. The latter will definitely be faster in upcoming v8 versions but probably not right now.

BridgeAR · 2017-09-11T04:21:10Z

@Gerhut I think it is fine to keep it in this PR but it would be just as fine to move it to another one!

If this is kept in the PR the commit message has to be updated though. This has to be done one way or the other anyway though since it currently has information that is not necessary for landing. So I think it is your call. If you want to land another commit - feel free to open a second PR for that.

bnoordhuis · 2017-09-11T10:02:53Z

I'm -0. process.env and the .env option currently behave the same: values are tostring-ed. This PR introduces a discrepancy for no good reason I can see.

Dimissed my ok as I am also -0 on the change but I do not want to block it either.

BridgeAR · 2017-09-22T21:03:14Z

This is semver major and needs some LGs @nodejs/tsc

It is still a tiny bit controversial in general though, so please read all the comments as well.

evanlucas · 2017-09-25T18:00:01Z

I'm on the fence on this one. Deviating from how null and undefined are handled in process.env makes this more confusing to me...

targos · 2017-09-25T18:02:44Z

I would be in favour of the same change for process.env

refack · 2017-09-25T18:51:14Z

test/parallel/test-child-process-env.js

-  'HELLO': 'WORLD'
+  'HELLO': 'WORLD',
+  'UNDEFINED': undefined,
+  'NULL': null


Could you add 'EMPTY': ''

refack · 2017-09-25T18:57:06Z

doc/api/child_process.md

@@ -412,7 +412,8 @@ Use `cwd` to specify the working directory from which the process is spawned.
 If not given, the default is to inherit the current working directory.

 Use `env` to specify environment variables that will be visible to the new
-process, the default is [`process.env`][].
+process, the default is [`process.env`][], `undefined` values in `env` will be


Suggestion keep the old line, and add a new sentence:

process, the default is [`process.env`][]. `undefined` values in `env` will be ignored.

addaleax · 2017-12-06T04:46:03Z

lib/child_process.js

-  for (var key in env) {
-    envPairs.push(`${key}=${env[key]}`);
+  for (const key of Object.keys(env)) {
+    const value = env[key]


Can you make sure running make lint passes?

At present, undefined values of env option will be transferred as a "undefined" string value, and values in prototype will also be included, which are not usual behaviors. Since non-string env values & prototype values are undocumented, this change may be treated as a bugfix or a breaking change. Tested on Mac, Windows not yet. Fixes: nodejs#15087

Gerhut · 2017-12-06T08:21:07Z

Sorry for only ran make test-parallel these times.

targos · 2018-01-15T10:25:45Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12531/

targos · 2018-01-15T11:32:55Z

Pushed a fix for Windows.
CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/14890/

At present, undefined values of env option will be transferred as a "undefined" string value, and values in prototype will also be included, which are not usual behaviors. This commit prevents those to be transferred to the environment of the child process. PR-URL: nodejs#15089 Fixes: nodejs#15087 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos · 2018-01-15T12:56:37Z

Landed in 85739b6.
Thank you @Gerhut for your patience and first contribution to Node!

MylesBorins · 2018-01-17T18:37:57Z

In future please run CITGM before landing semver major commits

edit: for context this broke CITGM

targos · 2018-01-17T19:26:24Z

Sorry about that

apapirovski · 2018-01-17T19:29:05Z

I'll just leave a note here to anyone landing this in the future, it can't land without #18210.

Before this commit, setting a property of process.env to undefined would set the value to the string "undefined". This changes the behavior to instead remove the value, working like the delete operator. Refs: nodejs#15089

PR-URL: #18990 Refs: #15089 Refs: #18158 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

PR-URL: nodejs#18990 Refs: nodejs#15089 Refs: nodejs#18158 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Aug 30, 2017

benjamingr reviewed Aug 30, 2017

View reviewed changes

benjamingr added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 30, 2017

cjihrig reviewed Aug 30, 2017

View reviewed changes

Gerhut force-pushed the master branch from fa5df44 to ea03fc8 Compare August 31, 2017 06:20

Gerhut force-pushed the master branch from ea03fc8 to 8c10ccd Compare September 4, 2017 02:59

BridgeAR previously approved these changes Sep 11, 2017

View reviewed changes

Gerhut force-pushed the master branch from 8c10ccd to d3e7468 Compare September 12, 2017 05:45

Gerhut changed the title ~~child_process: ignore null/undefined values of env~~ child_process: ignore undef/proto values of env Sep 12, 2017

jasnell approved these changes Sep 15, 2017

View reviewed changes

Gerhut force-pushed the master branch from d3e7468 to c3be4f7 Compare September 18, 2017 06:24

refack reviewed Sep 25, 2017

View reviewed changes

Gerhut force-pushed the master branch from c3be4f7 to cccd9d8 Compare September 27, 2017 05:26

BridgeAR added the tsc-review label Dec 6, 2017

BridgeAR requested a review from a team December 6, 2017 03:33

addaleax approved these changes Dec 6, 2017

View reviewed changes

Gerhut force-pushed the master branch from e566e1b to 5e37647 Compare December 6, 2017 08:19

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

rvagg mentioned this pull request Dec 19, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-12-20 nodejs/TSC#447

Closed

targos approved these changes Jan 15, 2018

View reviewed changes

fixup: fix test for Windows

e7c8d07

targos closed this Jan 15, 2018

targos mentioned this pull request Jan 15, 2018

src: delete process.env values set to undefined #18158

Closed

4 tasks

Trott removed the tsc-review label Jan 16, 2018

TimothyGu mentioned this pull request Feb 25, 2018

process: doc-only deprecate non-string env value #18990

Closed

4 tasks

		@@ -467,7 +467,10 @@ function normalizeSpawnArguments(file, args, options) {
		var envPairs = [];

		for (var key in env) {

child_process: ignore undef/proto values of env #15089

child_process: ignore undef/proto values of env #15089

Conversation

Gerhut commented Aug 30, 2017 • edited Loading

Checklist

Affected core subsystem(s)

benjamingr Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Aug 30, 2017

benjamingr commented Aug 30, 2017

cjihrig left a comment

Choose a reason for hiding this comment

addaleax commented Aug 31, 2017

BridgeAR commented Aug 31, 2017 • edited Loading

jasnell commented Sep 1, 2017

targos commented Sep 1, 2017

addaleax commented Sep 2, 2017

BridgeAR commented Sep 3, 2017

refack commented Sep 3, 2017

Gerhut commented Sep 4, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 11, 2017

bnoordhuis commented Sep 11, 2017

BridgeAR commented Sep 22, 2017

evanlucas commented Sep 25, 2017

targos commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerhut commented Dec 6, 2017

targos commented Jan 15, 2018

targos commented Jan 15, 2018

targos commented Jan 15, 2018

MylesBorins commented Jan 17, 2018 • edited Loading

targos commented Jan 17, 2018

apapirovski commented Jan 17, 2018

Gerhut commented Aug 30, 2017 •

edited

Loading

benjamingr Aug 30, 2017 •

edited

Loading

BridgeAR commented Aug 31, 2017 •

edited

Loading

MylesBorins commented Jan 17, 2018 •

edited

Loading